Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add renderProp to ShallowWrapper #1863

Merged
merged 3 commits into from
Nov 5, 2018
Merged

Add renderProp to ShallowWrapper #1863

merged 3 commits into from
Nov 5, 2018

Conversation

dferber90
Copy link
Contributor

@dferber90 dferber90 commented Oct 11, 2018

Adds a renderProp method to ShallowWrappers to ease testing render props.

This PR basically extracts renderProp out of @commercetools/enzyme-extensions. We would deprecate and drop it there if renderProp makes it into enzyme itself.

I wrote this article a while ago detailing how to use renderProp and why.

I'm curious for feedback of whether you'd want to accept it or not after some interest was shown in commercetools/enzyme-extensions#8.


Thanks to @tdeekens who helped set up and bounce ideas back&forth with for renderProp in enzyme-extensions!

Closes #1444. Related to #1856.

@dferber90 dferber90 changed the title [New] : add renderProp [New] : add renderProp to ShallowWrapper Oct 11, 2018
@dferber90 dferber90 changed the title [New] : add renderProp to ShallowWrapper Aadd renderProp to ShallowWrapper Oct 11, 2018
@dferber90 dferber90 changed the title Aadd renderProp to ShallowWrapper Add renderProp to ShallowWrapper Oct 11, 2018
@dferber90

This comment has been minimized.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Let's add this same API to mount, for consistency.

packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme/src/ShallowWrapper.js Outdated Show resolved Hide resolved
* @returns {ShallowWrapper}
*/
renderProp(propName, ...args) {
const Wrapper = props => props.children;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work in React 13.

packages/enzyme/src/ShallowWrapper.js Outdated Show resolved Hide resolved
throw new Error(`no prop called “${propName}“ found`);
}
if (typeof prop !== 'function') {
throw new Error(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be a TypeError

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to a TypeError.

I'm curious why you'd consider this one a TypeError, but not the one above as both are problems of the component's props?

}

const element = prop(...args);
const wrapped = getAdapter(this[OPTIONS]).createElement(Wrapper, null, element);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may need to add a method to all the adapters "wrap", so that the adapter itself can handle the wrapper implementation.

Copy link
Contributor Author

@dferber90 dferber90 Oct 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to do it in this PR or separately? Separately I assume, just double-checking.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd need to be in this PR (in a separate commit, ofc).

Copy link
Contributor Author

@dferber90 dferber90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to add wrap to all adapters but I'm not familiar enough with the internals of enzyme to get the tests to pass. I still keep bumping into TypeError: inst.render is not a function on React 14 😞 Other versions of React seem to be passing the tests now though!

Let's add this same API to mount, for consistency.

I'm holding off adding renderProp to mount until the tests are passing in shallow. I haven't used mount myself yet either. I thought mount renders the whole tree, in which case the render prop would get executed automatically? I guess I need to play with mount a bit to understand why/how it would be useful there.

packages/enzyme/src/ShallowWrapper.js Outdated Show resolved Hide resolved
throw new Error(`no prop called “${propName}“ found`);
}
if (typeof prop !== 'function') {
throw new Error(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to a TypeError.

I'm curious why you'd consider this one a TypeError, but not the one above as both are problems of the component's props?

@dferber90
Copy link
Contributor Author

I tried to add wrap to all adapters but I'm not familiar enough with the internals of enzyme to get the tests to pass. I still keep bumping into TypeError: inst.render is not a function on React 14 😞 Other versions of React seem to be passing the tests now though!

The issue was const stub = sinon.stub().returns(null); effectively rendering null from a stateless functional component, which is not supported in React 14. Changing it to const stub = sinon.stub().returns(<div />); solved the issue. This comment tipped me off.

@ljharb
Copy link
Member

ljharb commented Oct 20, 2018

I've updated and rebased the branch; I'll give it another review and then we can add the method for mount as well.

@dferber90
Copy link
Contributor Author

I'm still a bit puzzled by how renderProp would work with mount - or what it would be useful for (I haven't actually used mount myself). Depending on the system under test, the renderProp will likely get called anyways during rendering, so users can test the final output.

The only useful case for having renderProp in mount I could come up with would be to influence the arguments the actual render-prop gets called with. But there should be other means to do so by setting the parent component up properly. I'm questioning how useful having renderProp in mount would be after all.

I attempted to add mount anyways, but I failed at coming up with a good example for docs/tests and at actually making the implementation work. I'm not sure how to proceed as I feel kinda lost without understanding the end-goal of having renderProp in mount.

It would be nice if you could give me some direction or take over from here.

@ljharb
Copy link
Member

ljharb commented Oct 21, 2018

It's so you can unit-test the render prop without needing to know all the permutations of props that lead to certain input combinations.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and added mount support, as well as some unit tests for adapters' wrap method.

This looks good to go, thanks!

@ljharb ljharb merged commit 6cdd27c into enzymejs:master Nov 5, 2018
@ljharb ljharb mentioned this pull request Nov 5, 2018
41 tasks
@dferber90
Copy link
Contributor Author

@ljharb Thanks for driving this home!

I visited a friend this weekend so I didn't get to continue working on it. I really appreciate your help!

We'll deprecate the renderProp function in @commercetools/enzyme-extensions once this is released.

@dferber90
Copy link
Contributor Author

The current API works like this wrapper.renderProp(propName, arg1, arg2).

This will call the prop propName on wrapper with arg1 and arg2, like wrapper[propName](arg1, arg2).

This API is nice to work with but prevents us from ever adding something like an options object or any other param to renderProp itself, as all arguments after the first one get forwarded to the render prop.

It might be a better idea to use an API like wrapper.renderProp(propName, [arg1, arg2]) or wrapper.renderProp(propName)(arg1, arg2).

So far this has not been a problem for us but I wanted to give a heads-up now that it's becoming official. I'm undecided of which API is suited best.

@ljharb
Copy link
Member

ljharb commented Nov 6, 2018

@dferber90 those are really good points. The explicit array is cleaner, until you want no arguments and you want to pass options. The “return a function” approach is cleaner when you have arguments, but perhaps less so when you don’t.

I think I’m slightly leaning towards the “returning a function” API. I’ll be releasing the adapters sooner than enzyme itself, so we have a bit of time to make the change, but not much :-)

Copy link
Contributor Author

@dferber90 dferber90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb I reviewed your latest changes and found a left-over console.log and a mistake in the docs I made. I opened #1890 which fixes them.

```jsx
const wrapper = mount(<App />)
.find(Mouse)
.renderProp('render', [10, 20]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is under "multiple arguments", so the code should be .renderProp('render', 10, 20)

```jsx
const wrapper = shallow(<App />)
.find(Mouse)
.renderProp('render', [10, 20]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, this should also be .renderProp('render', 10, 20).

)();

export default function wrap(element) {
console.log(React.version, Wrapper);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a debugging-leftover? This should go away as far as I can tell.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops, yes, that's my bad.

@dferber90 dferber90 mentioned this pull request Nov 6, 2018
@dferber90
Copy link
Contributor Author

@ljharb I now also opened #1891 to suggest the "return a function" approach instead.

ljharb added a commit that referenced this pull request Nov 8, 2018
 - [new] add `wrapWithSimpleWrapper` (#1863, #1890)
ljharb added a commit that referenced this pull request Nov 8, 2018
 - [new] `wrap`: add `wrap` to all adapters (#1863)
 - [deps] update `enzyme-adapter-utils`, `react-is`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment